-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add cached, named Visibility profiles #5074
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
To benchmark this, I modified schema = Class.new(ProfileLargeResult::Schema) do
# def self.visible?(member, context)
# sleep 0.001
# super
# end
end
schema2 = Class.new(schema) {
use GraphQL::Schema::Visibility
}
document = ProfileLargeResult::ALL_FIELDS
Benchmark.ips do |x|
x.config(time: 10)
x.report("[WARDEN] Querying for #{ProfileLargeResult::DATA.size} objects") {
schema.execute(document: document)
}
x.report("[PROFILE] Querying for #{ProfileLargeResult::DATA.size} objects") {
schema2.execute(document: document)
}The performance was about the same between the two (Warden a bit faster? within the margin of error.), except when a cached visibility profile was used and Visibility::Profile uses a hair less memory (less than 1% difference). |
| - `Visibility` supports predefined, reusable visibility profiles which speeds up queries using complicated `visible?` checks. | ||
| - `Visibility` hides types differently in a few edge cases: | ||
| - Previously, `Warden` hide interface and union types which had no possible types. `Visibility` doesn't check possible types (in order to support performance improvements), so those types must return `false` for `visible?` in the same cases where all possible types were hidden. Otherwise, that interface or union type will be visible but have no possible types. | ||
| - Some other thing, see TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @rmosolgo – could you provide any more detail here? This memory would really help finding the horcruxes :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Derp... sorry! I think it must have to do with this: f591d98
I'm going to take a stroll down memory lane and if I can remember or rediscover the issue, I'll update this doc and follow up here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I updated the note here: 4d325ef
I think it's possible that this compatibility issue could be resolved now, because Schema::Visibility does have a top-level map of types, created by this code here:
graphql-ruby/lib/graphql/schema/visibility.rb
Line 203 in 4d325ef
| def load_all(types: nil) |
If it looks like your schema is affected by this issue, let me know and I'll give it a shot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Invaluable context, thanks for the quick reply. I'm going to read that new sentence on repeat this afternoon until it clicks. Also, my gosh – abstracts are hard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha! If I've understood it properly, a minimal situation is like this:
class Thing < GraphQL::Schema::Object
implements Thingable
end
module Thingable
include GraphQL::Schema::Interface
field :f, String
end
class SomeUnion < GraphQL::Schema::Union
possible_types(Thing)
def self.visible?(...); false; end
end
class Query < GraphQL::Schema::Object
field :thingable, Thingable
field :some_union, SomeUnion
end Going from old to new, I think the difference in visible schema would be this:
type Query {
thingable: Thingable
}
interface Thingable {
f: String
}
- type Thing implements Thingable {
- f: String
- }Visibility wouldn't check SomeUnion and discover Thing -- but Warden would, because it used the top-level map from Schema.types. To acheive compatibility, you'd have to add:
module Thingable
# ...
orphan_types(Thing)
end Or add field :thing, Thing to Query -- just some way to discover Thing while traversing visible members of the schema.
But this might be fixable now...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ie: anything that's not the interface membership?
Yes, that's what I meant by that ... sorry for the frenzied terseness :P
orphan_types is specifically for the case when an object type is only used in the schema as an implementer of an interface. It's a practical consideration because, in Rails development, we need Rails to load the Object type class so that GraphQL-Ruby can know about the interface implementation. Otherwise, the file might never be loaded.
For example, in a schema like this:
type Query {
node(id: ID): Node
}
interface Node {
id: ID
}
type Thing implements Node {
id: ID
}GraphQL-Ruby would need Node.orphan_types(Thing).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last question then... how does Schema.orphan_types differ from Interface.orphan_types? I haven't always used the later but haven't noticed unexpected side effects. Is schema orphan types all-encompassing? Thanks again, this is all incredibly helpful (both for visibility upgrade and gem projects).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The two orphan_types methods are basically the same: just ways to attach things to the schema that might not be attached. I don't know of any differences between them 😅 IIRC the gem doesn't even require than an Interface's orphan types implement that interface ... Yes, always using Schema.orphan_types is a fine approach -- Schema.from_definition just passes all object types to that method!
| orphan_types(object_types) |
Happy to help 🍻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Schema.from_definition proceeds to do the same work again with the interfaces themselves directly below. I think that could be removed entirely (I omitted it from the stitching composer and it seems to work?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good catch -- I'll try removing it in #5269
This aims to add to improvements to GraphQL-Ruby's (forthcoming) schema visibility system.
.visible?during execution)dynamic: true, which is how the current visibility system always works, calling.visible?for every schema member on every query.Part of #5014
TODO
preload: true(defaults to true whenRails.env.production?)Accept configuredvisible_in: ...or similar, for named visibility profilesVisibility::Subset=>Visibility::ProfileVisibility::Profile